This is true for Status requests, but not true for Enhancement requests which subsume Status.

Yes. I noticed that and the first commit of nuttycom#39 (nuttycom@239f7ff) fixes it. The implementation is fine, it's just the comment that is incorrect.

It occurs to me that https://github.com/zcash/librustzcash/pull/1473/files#diff-a1724f7547ecabc613a07520eaf7917d9084d4277219d186698b0c8e352495a4R2425-R2427 is superfluous; we should never have a Status request for a transaction unless we have the raw data. So one way to fix this would be to remove the query type argument from the queue_tx_retrieval function, and to set it based upon the presence or absence of raw transaction data.

The comment change doesn't resolve my concern. GetStatus is indeed only generated when we have the raw transaction (as I noted), but Enhancement is generated by definition when we don't have the raw transaction, and its documentation states that when the caller is unable to look up the transaction, it must call this method.

The reason it is okay to delete for Enhancement requests is that:

• If a transaction can't be enhanced, we assume that the caller calls this method with TransactionStatus::TxidNotRecognized (as the other two statuses mean that the chain data source knew about the transaction, and therefore must have observed it and could return its raw data), so we don't need to consider what happens in the other statuses.
• We only delete for TransactionStatus::TxidNotRecognized if the transaction is expired, at which point we don't necessarily care about enhancement (though the user might still care about the memo), and it is very likely that we will be unable to ever enhance (as the tx will have been dropped from all mempools).

Simplified in 5a1645a

The fact is that an Enhancement request is a Status request. That's what it means for an Enhancement request to subsume a Status request. It's either "get me the status" or "get me the status and the data if it's available".

@daira Enhancement is getting the status; it subsumes / is a superset of GetStatus. That's why I raised my initial concern that this method was being written ostensibly assuming it would only be called via GetStatus, which was false.

As @nuttycom says, the actual implementation does correctly work with both GetStatus and Enhancement; the comment was just misleading.

I have now updated this comment again. (b47c7bf)

The fact is that an Enhancement request is a Status request.

Not so. Otherwise it wouldn't have to requeue a Status request when queue_status_retrieval is true. It doesn't directly update the mined height in the case where d_tx.mined_height().is_none().

Otherwise it wouldn't have to requeue a Status request when queue_status_retrieval is true.

The re-queueing is so that the wallet continues to monitor for when the transaction is eventually mined. A relevant case here is that store_decrypted_tx was called with a transaction that was read from the mempool; we have complete transaction data, but we don't know yet whether the transaction will be mined, so we stick the status request back on the queue. For a fully transparent transaction that we get from the mempool we have no other way of detecting that it becomes mined.